Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix routing inconsistencies #10

Closed
wants to merge 1 commit into from
Closed

Fix routing inconsistencies #10

wants to merge 1 commit into from

Conversation

marcbachmann
Copy link
Contributor

@marcbachmann marcbachmann commented Dec 28, 2017 •
edited
Loading

Currently the normalization of the route only happens when the curry option is in use.
With this commit, that behavior is also applied when that's disabled.

This might be a breaking change in which case we could ship it with the removal of the curry option. But sadly I have no idea what that normalization is used for.
Therefore I also can't write tests 😢 .

edit: I just found one use case for that normalization, so it looks like this is a bug... but theoretically still breaking. Here's a choo example that uses #active as route:
https://github.com/choojs/choo/blob/master/example/index.js#L14

app.route('/', require('./view'))
app.route('#active', require('./view'))
app.route('#completed', require('./view'))
app.route('*', require('./view'))

@marcbachmann
Copy link
Contributor Author

I'd say we should bump by a major version and advise people to use wayfarer directly if they don't want the route transforms.

@yoshuawuyts yoshuawuyts mentioned this pull request Dec 28, 2017
Open
@yoshuawuyts
Copy link
Member

Hey, this is great!

Yeah I think we should just remove the curry option. Probably also do all the other things in #12 and release as a semver major. What do you think?

@marcbachmann
Copy link
Contributor Author

Probably also do all the other things in #12 and release as a semver major. What do you think?

👍 I'll create a new pr with a suggestion how to migrate from the old version. I'd still keep .match, .emit, .on aka .route. .emit is quite small and then the api will be the same as wayfarer.

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Dec 29, 2017 •
edited
Loading

@marcbachmann I was thinking, do we really need both .match() and .emit() - if we're going for a breaking change, it might make sense to only keep a single API. What do you think?

Perhaps rename .match() to .emit() instead?

edit: nah, I think you're right. Having both .match and .emit makes sense!

@marcbachmann
Copy link
Contributor Author

marcbachmann commented Dec 29, 2017 •
edited
Loading

We'll fix that in #13

@marcbachmann marcbachmann deleted the fix-routing-inconsistencies branch December 29, 2017 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marcbachmann @yoshuawuyts